Refine temperature/top_p handling for reasoning models#2277
Refine temperature/top_p handling for reasoning models#2277mayeco wants to merge 4 commits intoOpenHands:mainfrom
Conversation
Update handling of temperature and top_p for OpenAI models.
There was a problem hiding this comment.
Pull request overview
Updates chat-completion option normalization to avoid stripping sampling params (temperature, top_p) for non-OpenAI “reasoning effort” models (notably Gemini), while still handling OpenAI reasoning-model quirks.
Changes:
- Only remove
temperature/top_pfor OpenAI reasoning models (o1*,o3*) instead of all models that supportreasoning_effort. - Add model-name normalization logic in
select_chat_optionsto gate this behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openhands-sdk/openhands/sdk/llm/options/chat_options.py:59
- The Gemini 2.5-pro substring check is case-sensitive and bypasses the
model_lowernormalization introduced above. For consistency with other model checks in this function (and withmodel_features.model_matches, which is case-insensitive), usemodel_lower(or another normalized form) so variants like provider-prefixed or differently-cased model IDs don’t skip this defaulting behavior.
# Gemini 2.5-pro default to low if not set
if "gemini-2.5-pro" in llm.model:
if llm.reasoning_effort in {None, "none"}:
out["reasoning_effort"] = "low"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| model_lower = llm.model.lower() | ||
| # Normalize to basename so provider-prefixed IDs like "openai/o1" are handled | ||
| model_name = model_lower.split("/")[-1] | ||
| if model_name.startswith(("o1", "o3")): |
There was a problem hiding this comment.
I think it’s not only o1, o3, there are also Claudes, open models, GPTs
I’d actually suggest that since we removed temperature by default, we could remove this temperature special cases for all
The other models will also get nothing set (from the SDK), so they all should respect user choice
|
yes I understand @enyst but #1989 but still not fix my issue. If I have a reasoning model like gemini, then set the temperature to 0.0. then will be 1 (the default) |
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Oh, I agree. I just don't fully understand, why keep the code for o1 and o3?
|
yes. maybe is not only o1 / o3 we need to make a list of models that the reasoning fail when temperature is set. |
|
Sounds good! |
Update handling of temperature and top_p for OpenAI models.
This fix issue #2278
Checklist